Add a new requirement function to disable dangerous functions#23141
Add a new requirement function to disable dangerous functions#23141orthagh wants to merge 7 commits intoglpi-project:11.0/bugfixesfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new security requirement class DangerousFunctionsSecurity that checks whether potentially dangerous PHP functions are enabled. The requirement is integrated into both the system requirements manager and the Central dashboard to warn administrators about security risks.
Changes:
- Added
DangerousFunctionsSecurityrequirement class that checks for 33 potentially dangerous PHP functions - Integrated the new requirement into
RequirementsManagerfor system checks - Integrated the new requirement into
Centraldashboard to display security warnings
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Glpi/System/Requirement/DangerousFunctionsSecurity.php | New requirement class that validates dangerous PHP functions are disabled |
| src/Glpi/System/RequirementsManager.php | Adds DangerousFunctionsSecurity to optional requirements list |
| src/Central.php | Adds DangerousFunctionsSecurity to security requirements shown in Central dashboard |
cedric-anne
left a comment
cedric-anne
left a comment
There was a problem hiding this comment.
Some posix_ functions can be dangerous too.
orthagh
force-pushed
the
feat/disablefunction
branch
from
f3fa830 to
e4c7fb6
Compare
List updated from wordpress recommendations (from a tier party website |
|
Is it right to enable it for CLI? Some configurations might have a different .ini config for CLI and web. I think our own docker image allow these methods on the CLI but not on the web (at least this is what I heard last week, unsure if it was merged that way). |
Asked by @cedric-anne in #23141 (comment) |
| 'pcntl_alarm', | ||
| 'pcntl_fork', | ||
| 'pcntl_waitpid', | ||
| 'pcntl_wait', | ||
| 'pcntl_wifexited', | ||
| 'pcntl_wifstopped', | ||
| 'pcntl_wifsignaled', | ||
| 'pcntl_wifsignaled', | ||
| 'pcntl_wifcontinued', | ||
| 'pcntl_wexitstatus', | ||
| 'pcntl_wtermsig', | ||
| 'pcntl_wstopsig', | ||
| 'pcntl_get_last_error', | ||
| 'pcntl_strerror', | ||
| 'pcntl_sigprocmask', | ||
| 'pcntl_sigwaitinfo', | ||
| 'pcntl_sigtimedwait', | ||
| 'pcntl_exec', | ||
| 'pcntl_getpriority', | ||
| 'pcntl_setpriority', | ||
| 'posix_ctermid', | ||
| 'posix_getcwd', | ||
| 'posix_getegid', | ||
| 'posix_getgid', | ||
| 'posix_getgrgid', | ||
| 'posix_getgrnam', | ||
| 'posix_getgroups', | ||
| 'posix_getlogin', | ||
| 'posix_getpgid', | ||
| 'posix_getpgrp', | ||
| 'posix_getpid', |
There was a problem hiding this comment.
Maybe it could rely on get_defined_functions() + preg_grep() to get all pcntl/posix functions. It could permit to be sure that no function is forgotten and any new function introduced in the future would be detected.
| } | ||
| } | ||
| $this->validation_messages[] = sprintf( | ||
| __('Functions "%s" are enabled. Please disable them in php.ini (see disable_functions directive) to avoid security risks.'), |
There was a problem hiding this comment.
It can be disabled either from the php.ini directive or directly in the webserver configuration.
| __('Functions "%s" are enabled. Please disable them in php.ini (see disable_functions directive) to avoid security risks.'), | |
| __('Functions "%s" are enabled. You should disable them (see `disable_functions` PHP configuration directive) to avoid security risks.'), |
There was a problem hiding this comment.
Not sure about this one. It's clearer for people that don't know how to do it and for other we probably don't have to explain
I know, just saying that our own docker image might not validate this requirement. |
Co-authored-by: Cédric Anne <cedric.anne@gmail.com>
It is not a problem if our development image does not fit this new requirement in CLI context, as it is optional. Our production docker image will be updated to fit them. |
|
|
|
Should we really consider sending mail from sendmail ins unsafe? |
The problem is that it prevents to disable the |
| 'socket_accept', | ||
| 'socket_bind', | ||
| 'socket_clear_error', | ||
| 'socket_close', | ||
| 'socket_connect', | ||
| 'socket_listen', | ||
| 'socket_create_listen', | ||
| 'socket_read', | ||
| 'socket_create_pair', |
There was a problem hiding this comment.
I'm not sure we should block them.
Socket could be use for example by plugin to interact with a realtime tool.
|
A new PHPStan rule has been created based on the configuration here. I've run it locally and our codebase (for glpi 11.x) don't have call to those forbidden tools |
If someone has an idea to test the thing, please suggest it.
(as the thing can only be enabled in php.ini)